Skip to content

fix: Reject deleted flags and segments in flag overrides - #574

Open
kinyoklion wants to merge 4 commits into
feat/overridesfrom
rlamb/overrides-python-reject-deleted
Open

kinyoklion wants to merge 4 commits into
feat/overridesfrom
rlamb/overrides-python-reject-deleted

Conversation

@kinyoklion

@kinyoklion kinyoklion commented Nov 10, 2026 •

Copy link
Copy Markdown
Member

Requirements

  • I have added test coverage for new or changed functionality
  • I have followed the repository's pull request submission guidelines
  • I have validated my changes against all supported platform versions (run on Python 3.13 only; CI does not run for this base)
  • Sync/async parity: matching changes made to async_* siblings (or N/A). The fix is in the override layer and sink, which LDClient and AsyncLDClient share. The tests cover both clients.

Related issues

Describe the solution you've provided

Before this change, a flag or segment with "deleted": true went into the override layer, from a custom source or from an override file. The file source counted the entry as an override in effect. The overlay served it, and variation() and variation_detail() raised AttributeError: _track_events into the application, in both LDClient and AsyncLDClient. A flag that depended on it evaluated with reason EXCEPTION and without overrideAffected.

After this change:

  • Sink and layer (1.1.1.1, 1.2.8.1). When a snapshot contains a flag or segment marked as deleted, as a dictionary or a model object, the sink rejects the whole snapshot. The layer is unchanged and no change notifications are sent. The rejection is logged at Error level and names the kind and the key, for example Rejected an override snapshot; the overrides in effect stay: flag "f" is marked as deleted; an override is removed by leaving it out of the snapshot. set_overrides raises ValueError, as it already does for an invalid definition, so the data system handles both the same way. If a source lets the error propagate out of start, the existing start-failure handling applies: the error is logged, the source is closed, and the client runs without overrides. A rejected later update keeps the overrides in effect. The error goes back to the source's own caller.
  • File override source (1.3.6.2). The source checks for deleted entries when it parses each file, before it calls the sink. The reload fails the same way as for a file that cannot be parsed. The previous overrides stay, and the failure is logged like other load failures, naming the file and the key: Unable to load flag data: flag "f" in /path/overrides.json is marked as deleted; remove the entry to remove its override. The existing retry applies.
  • An entry with "deleted": false is an ordinary definition and still loads.
  • Docs. FileOverrideSourceBuilder and OverrideSink.set_overrides each get one user-level sentence.

The ordinary file data sources are unchanged on purpose. Files.new_data_source and Files.new_data_source_v2 do not use the override source's parsing code, and they still accept deleted entries. Rejecting deleted entries there would be a breaking change, so it is left for a later major version. New tests in test_file_data_sources_pinned_behavior.py pin the current behavior: the FDv1 source stores such entries as deleted, and the FDv2 source passes them on unchanged.

Tests

  • testing/impl/test_overrides.py: the layer and the sink reject a deleted flag and a deleted segment, each as a dictionary and as a model, with and without listeners. In every case the layer is unchanged, nothing is notified, exactly one Error is logged, and the next snapshot is accepted. A definition with deleted: false is accepted.
  • testing/test_ldclient_overrides.py and testing/test_async_client_overrides.py use a custom source. If it supplies a deleted flag at start, the client serves LaunchDarkly data for the flag and its dependent. If it supplies one in a later update, ValueError is raised to the source, the override and the dependent's overrideAffected reason stay, and nothing is notified. In neither case do variation() or variation_detail() raise.
  • testing/integrations/test_file_override_source.py:
    • An initial load with a deleted flag or segment fails, in JSON and in YAML, and applies none of the other file's overrides.
    • A reload with a deleted entry keeps the previous overrides and logs the file and the key. The retry then picks up the fix with no change signal.
    • deleted: false loads.
    • A client with such a file serves no override.
  • testing/integrations/test_filedata.py: load_files and read_file raise DeletedEntryError, a FileReadError, and its message names the file and the key. deleted: false loads.
  • testing/integrations/test_file_data_sources_pinned_behavior.py: pins that the ordinary data sources are unchanged.

I ran every rejection test against the base commit without the fix, and each one fails there. The deleted: false tests and the pins pass both with and without the fix, as expected.

How I ran them, with the clone's venv and this worktree on PYTHONPATH:

python -m pytest -q -p no:cacheprovider ldclient --ignore-glob='*dynamodb*' --ignore-glob='*redis*' --ignore-glob='*consul*'
# 2142 passed
mypy ldclient                                    # no issues
isort --check --atomic ldclient contract-tests   # clean
pycodestyle ldclient contract-tests              # clean

Describe alternatives you've considered

  • Dropping only the deleted entry and applying the rest. The spec requires rejecting the whole snapshot, so that the layer always holds exactly one snapshot that a source supplied.
  • Serving a deleted entry as an absent flag. That would hide LaunchDarkly data for the key, and overrides only add or replace definitions.

Additional context

Python CI runs only for PRs based on main or feat/**, so it does not run on this PR.

Opened from termag


Note

Overview
Override snapshots and file-based override loads now reject any flag or segment with "deleted": true, instead of accepting them into the override layer (which could break evaluation).

File parsing (filedata) adds DeletedEntryError and _reject_deleted_entries, run after parse in read_file / _parse_file, so a reload with a deleted entry fails like a parse error and prior overrides stay in effect.

Override application adds DeletedItemError in OverrideLayer.set_all; OverrideSinkImpl logs and re-raises so nothing is applied and listeners are not notified. Public docs on FileOverrideSourceBuilder and OverrideSink.set_overrides state that removal is done by omitting the entry, not by marking it deleted.

Legacy file data sources (FDv1/FDv2) are unchanged and still accept deleted entries; new pinned tests document that behavior.

Tests cover layer, sink, sync/async clients, file override source (JSON/YAML), and load_files.

Reviewed by Cursor Bugbot for commit c6dcabc. Bugbot is set up for automated code reviews on this repo. Configure here.

@kinyoklion

Copy link
Copy Markdown
Member Author

bugbot review

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit b494eb1. Configure here.

kinyoklion added a commit that referenced this pull request Nov 10, 2026
<!-- TERMAG_PR_BODY_BEGIN -->
## Summary

This is the third step of the flag overrides port described by the
OVERRIDE specification. It is based on the model marker branch because
the phases are stacked; retarget to `feat/overrides` once that branch
merges.

**The override layer** (`ldclient/impl/overrides`). `OverrideLayer` is
the override store: it holds marked shallow copies of the flag and
segment definitions a source supplies, keyed by key, and is replaced
wholesale on each update, so the layer's contents are exactly one
snapshot at any instant and an empty snapshot clears it. Dictionaries
are decoded with the model constructors, and an invalid definition
raises rather than being defaulted. A definition whose key differs from
its key in the snapshot raises too, and the layer is left unchanged:
evaluation reads an entry by its snapshot key, while the all-flags state
and events use the definition's key. `OverrideStoreView` is the overlay
at the store read boundary: a read of a flag or segment returns the
override entry when one exists and the LaunchDarkly entry otherwise,
whatever the state of the base store, and an enumeration is the union of
both with the override entry winning, including over a deleted item. A
failure of the base enumeration propagates, as it does without the
overlay. When that read fails, `all_flags_state` logs the failure as
before and, if the override layer holds flags, returns a state of those
flags marked invalid rather than an empty one: overrides only add to the
state, and a client-side SDK bootstrapped from it uses its flags
whatever `$valid` says, so the overridden values still reach it. With no
override flags the state is the same invalid, empty state as without a
source. The sink stops accepting snapshots when the client is closed,
and when the source fails to start it also clears what the source
supplied. `OverrideSinkImpl` applies each snapshot as one serialized
operation and notifies flag change listeners of every flag whose
merged-view evaluation may have changed: entries added, removed, or
changed, plus every flag that depends on them through prerequisites or
segment references. An identical snapshot notifies nothing.

**LaunchDarkly changes reach override dependents.** The layer keeps a
dependency graph of its entries, rebuilt on each replacement, and the
store (sync and async) expands every changed LaunchDarkly item through
both its own graph and the layer's, to a fixpoint. A change to a
prerequisite flag or a segment that an override entry references
therefore notifies that entry's flag, the same way dependent flags are
notified for LaunchDarkly data, and a chain may cross between
LaunchDarkly data and overrides any number of times. Without an override
source the store's behavior is unchanged.

**Configuration and wiring.** `OverrideSource` and `OverrideSink` are
public protocols in `ldclient.interfaces`, so a custom source can be
supplied. `OverrideSourceBuilder` and `DataSystemConfig.override_source`
(also on `AsyncDataSystemConfig`) plus `ConfigBuilder.overrides(...)`
make the source an option of the FDv2 data system. The source is built
with the data system, so invalid configuration raises from the client
constructor like any other invalid component configuration. It starts
before the data system's own threads and its initial load completes
before the constructor returns; it is closed when the client is closed,
after the sink stops accepting snapshots, and an exception from its
`close` is logged. An exception from its `start` does not fail the
constructor or `start`: the data system logs it, closes the source,
clears any snapshot the source supplied before failing, and the client
runs without overrides. The SDK's own file-based source does not raise
from `start`; a custom source could. The source is not built or started
when the client is offline, and the SDK logs a warning that it is not
used. The data system's `store` becomes the overlay when a source is
configured, so evaluation, prerequisite and segment resolution, and the
all-flags read all see override precedence. The override source has no
effect on initialization status, data availability, or data source
status. The legacy FDv1 data system reports no override source. When the
data system fails to start, the client constructor stops each component
in turn and logs any failure to stop, so the event processor still stops
and the caller receives the start failure.

**The client facade.** Both `LDClient` and `AsyncLDClient` consult the
override layer before the not-initialized short-circuit: an overridden
flag is served before the client has LaunchDarkly data, and a flag the
layer does not hold takes exactly the path it took before this feature,
so a configured but empty source changes nothing with the same warning
and event as before. The all-flags state reads through the overlay.
While the client has no LaunchDarkly data it contains only the flags the
layer holds, read from the layer rather than the overlay so that
LaunchDarkly data in a store without an init marker stays out (with a
once-per-client warning), and it is invalid when the layer holds no
flags. Without a source this state is empty, because the SDK reports no
flags before the store is initialized, so the override flags only add to
what it reports; overrides never remove a flag from the state. An
evaluation that fails with an unexpected exception reports the
`EXCEPTION` reason with `overrideAffected` when the flag came from the
override layer, in a single evaluation and in the all-flags state
(OVERRIDE 1.5.4); the marker is read through `is_override_definition`,
so the error path cannot fail on an unexpected store item. On the async
client, override change notifications are marshalled onto the event
loop, like every other change notification.

**The async client with a persistent store.** The async client works out
which flags an override change affects from the in-memory store, the
only store it can read synchronously (`async_fdv2.py`). While a
persistent store is the active store, a change to an override notifies
the overridden flags, but not LaunchDarkly flags that depend on them
through a prerequisite or a segment. The documentation of
`AsyncDataSystemConfig.override_source` says so.

**Left to later PRs in the stack.**
- `all_flags_state` still keeps `trackEvents`, `trackReason` and
`debugEventsUntilDate` for override-affected flags (OVERRIDE 1.4.1.1).
The events step, #528, clears them.
- An override entry marked as deleted is rejected by #574, which is
stacked on top of the stack (launchdarkly/sdk-specs#288).

**Tests.** Unit tests for the layer, overlay (sync and async), and sink;
client tests for the gate, source lifecycle, construction errors,
offline, precedence, prerequisite and segment overrides, all-flags
state, and flag change and flag value change listeners, for both
clients; and a runner for the OVERRIDE specification test vectors
(`ldclient/testing/testdata/override-vectors/vectors.json`) through the
full client stack. The vector runner's per-evaluation summary marker
assertion is added with the events step.

[SDK-3250](https://launchdarkly.atlassian.net/browse/SDK-3250)


[SDK-3250]:
https://launchdarkly.atlassian.net/browse/SDK-3250?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ

<!-- CURSOR_SUMMARY -->
---

> [!NOTE]
> **Overview**
> Adds **experimental flag/segment overrides** that can take precedence
over LaunchDarkly data at evaluation time, wired through FDv2 (sync and
async).
> 
> **Configuration:** Optional `override_source` on the data system
(`ConfigBuilder.overrides(...)`, `OverrideSourceBuilder`, public
`OverrideSource` / `OverrideSink` protocols). Offline clients skip
building/starting the source.
> 
> **Runtime:** An `OverrideLayer` holds snapshot replacements;
`OverrideStoreView` overlays the feature store so per-key reads prefer
overrides; `OverrideSinkImpl` applies updates and fans out **flag
change** notifications via merged-view dependency diffing. The store can
register the layer as **external dependencies** so LaunchDarkly
prerequisite/segment updates also notify affected override flags.
> 
> **Clients:** `LDClient` / `AsyncLDClient` read the override layer
**before** the not-initialized default path and adjust
**`all_flags_state`** to return override-only bootstrapping when LD data
is unavailable (with once-per-client warnings). Evaluation errors set
**`overrideAffected`** when the definition came from overrides. Sync
client **rolls back components** if data system start fails.
> 
> **Tests:** Layer/overlay/sink units, broad sync+async client
scenarios, store cross-graph notification, and OVERRIDE **spec vectors**
(`override-vectors/vectors.json`).
> 
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
0489514. Bugbot is set up for automated
code reviews on this repo. Configure
[here](https://www.cursor.com/dashboard/bugbot).</sup>
<!-- /CURSOR_SUMMARY -->

<!-- TERMAG_PR_BODY_END -->

<!-- termag: box=i-0575d1c54853058d7 project=Overrides -->
<sub>Opened from termag</sub>
@kinyoklion
kinyoklion force-pushed the rlamb/overrides-python-reject-deleted branch from b494eb1 to 874d21d Compare November 10, 2026 22:24
@kinyoklion
kinyoklion force-pushed the rlamb/overrides-python-file-source-paths branch from a1bda43 to 5981654 Compare November 10, 2026 22:24
FileOverrideSourceBuilder passed anything other than a str to list(), so a single pathlib.Path raised TypeError from the constructor. A str or an os.PathLike is now one path, and every path is converted with os.fspath. A path that does not name a string path raises TypeError.
A bare bytes or bytearray value is a single path, and a TypeError from os.fspath is replaced by the builder's own message, so every rejected path is named. The type hint is Sequence, so that type checkers reject a set, whose order is not fixed. Tests cover a path object whose string form differs from its path and every rejected value.
The override layer never holds a deleted item (OVERRIDE 1.1.1.1). A snapshot
that contains a flag or segment marked as deleted, as a dictionary or a model
object, is rejected whole: the layer is unchanged, no change notifications are
sent, the rejection is logged at Error level naming the kind and the key, and
the sink raises ValueError (1.2.8.1).

The file-based override source detects such an entry when it parses a file,
before it calls the sink, and fails the reload as for a file that cannot be
parsed: the previous overrides stay, the failure names the file and the key,
and the retry applies (1.3.6.2). An entry with "deleted": false still loads.

The ordinary file data sources do not use this code and keep accepting
deleted entries; tests pin that.
@kinyoklion
kinyoklion force-pushed the rlamb/overrides-python-reject-deleted branch from 874d21d to aaa1188 Compare November 10, 2026 23:29
@kinyoklion
kinyoklion force-pushed the rlamb/overrides-python-file-source-paths branch from 5981654 to 154b5b8 Compare November 10, 2026 23:29
Base automatically changed from rlamb/overrides-python-file-source-paths to feat/overrides November 10, 2026 23:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants